Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New hook: useSafeTimeout #19109

Closed
wants to merge 2 commits into from
Closed

New hook: useSafeTimeout #19109

wants to merge 2 commits into from

Conversation

ellatrix
Copy link
Member

Description

Moves the logic within withSafeTimeout to the new hook useSafeTimeout.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

ellatrix
ellatrix commented Dec 12, 2019
View reviewed changes
packages/compose/src/hooks/use-safe-timeout/index.js
export default function useSafeTimeout() {
const timeouts = useRef( [] );

function clearTimeout( id ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use useCallback for these?

@ellatrix ellatrix added the [Status] In Progress Tracking issues with work in progress label Dec 12, 2019
@ellatrix ellatrix changed the title New hook: newSafeTimeout New hook: useSafeTimeout Dec 12, 2019
youknowriad
youknowriad reviewed Dec 13, 2019
View reviewed changes
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair, I'm not sure whether we really need this hook. I feel this is just a Higher-order that was needed because of how class components work. I feel with hooks, we don't really need it.

@ellatrix
Copy link
Member Author

@youknowriad Could you elaborate? Why won't we need it?

@youknowriad
Copy link
Contributor

I feel when you use timeouts with hooks, you'll always do something like that;

useEffect(() =>  {
  const tid = setTimeout(dosomething);
  return () => clearTimeout(tid);
});

The timeout and its clearance are located in the same code, the chance to forget about this and the added code here is very small while in the case of a class Component or components without hooks, it's quite complex to achieve: you need refactor to a class component and use separate methods to trigger and clear the timeouts. All these are fixed with hooks.

@ellatrix
Copy link
Member Author

Ah, you're right. I was thinking of a use case in RichText where setTimeout wouldn't be in useEffect, but this seems very rare.

@ellatrix ellatrix closed this Dec 13, 2019
@ellatrix ellatrix deleted the try/use-safe-timeout branch December 13, 2019 10:57
@ZebulanStanphill ZebulanStanphill mentioned this pull request Jan 24, 2020
Merged
@aduth
Copy link
Member

aduth commented Jan 27, 2020

Ah, you're right. I was thinking of a use case in RichText where setTimeout wouldn't be in useEffect, but this seems very rare.

There's now another example of this (timers in event handler) at #19881, though I'd agree that it's still not yet (to me) at a point to need a higher-level solution, and if anything its absence could serve as a deterrent to an anti-pattern.

torounit added a commit that referenced this pull request May 21, 2022
@torounit
Change to not use withSafeTimeout. ref: #19109
8e6ca5d
@torounit torounit mentioned this pull request May 27, 2022
Merged
torounit added a commit that referenced this pull request Jun 2, 2022
@torounit
Change to not use withSafeTimeout. ref: #19109
5911062
torounit added a commit that referenced this pull request Jun 2, 2022
@torounit @mirka
Rewrite <FormTokenField> to functional component and Typescript. (#…
ef6c5e9 
…41216)

* refactor `<TokenInput>` to functional component and Typescript.

* Remove findRenderedComponentWithType as it does not work with functional components.

* use findRenderedDOMComponentWithTag instead of findRenderedComponentWithType

* convert `<Token>` to TypeScript

* type only import

* convert SuggestionsList to TypeScript

* Change to not use withSafeTimeout. ref: #19109

* Remove comment because tabIndex is not specified.

* add type for dom-scroll-into-view

* add form-token-field dir to include in tsconfig.json

* refactoring types.

* fix type.

* refactor FormTokenField to TypeScript and Functional Component

* refactor story

* allow set suggestions in stories.

* Use act where useEffect is relevant.

* separate state

* revert ts-nocheck

* remove onChange prop form storybook

* Update packages/components/src/form-token-field/token-input.tsx

Co-authored-by: Lena Morita <lena@jaguchi.com>

* use ComponentProps

* add type docs

* replace token-field-wrapper to tsx

* add doc comment for FormTokenField

* fix type docs

* Update packages/components/src/form-token-field/types.ts

Co-authored-by: Lena Morita <lena@jaguchi.com>

* Update packages/components/src/form-token-field/types.ts

Co-authored-by: Lena Morita <lena@jaguchi.com>

* Update packages/components/src/form-token-field/index.tsx

Co-authored-by: Lena Morita <lena@jaguchi.com>

* Update packages/components/src/form-token-field/index.tsx

Co-authored-by: Lena Morita <lena@jaguchi.com>

* Update packages/components/src/form-token-field/index.tsx

Co-authored-by: Lena Morita <lena@jaguchi.com>

* change variable and function name

* Fix useEffect to updateSuggestions().

* Disabled control for __experimentalValidateInput. Update type document.

* add changelog

* Update packages/components/src/form-token-field/index.tsx

Co-authored-by: Lena Morita <lena@jaguchi.com>

Co-authored-by: Lena Morita <lena@jaguchi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowriad youknowriad youknowriad left review comments

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@talldan talldan Awaiting requested review from talldan

Assignees
No one assigned
Labels
[Status] In Progress Tracking issues with work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ellatrix @youknowriad @aduth